Add list-outputs #248
Add list-outputs #248
Conversation
|
|
||
|
|
||
| class TestListOutputs: | ||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
In the pytest parameters below, I'm using the same workflow to test different options of list-output. This runs the same workflow multiple times (redundant). I'm thinking of maybe running the workflow once, saving the workflow id in a global variable then have a different function that would run through the different list-output command
| @@ -9,10 +9,11 @@ | |||
| from cromshell.utilities import cromshellconfig | |||
|
|
|||
|
|
|||
| def run_cromshell_command(command: list, exit_code: int): | |||
| def run_cromshell_command(command: list, exit_code: int, options: list = None): | |||
There was a problem hiding this comment.
Let's document what these parameters are.
There was a problem hiding this comment.
done, also changed name to subcommand_options
| command_with_options = command.copy() | ||
| if options: | ||
| for item in options: | ||
| command_with_options.insert(1, item) |
There was a problem hiding this comment.
what if the options are meant to be in order? This seems to reverse that.
There was a problem hiding this comment.
updated, to take this into account
tests/unit/test_list_outputs.py
Outdated
| class TestListOutputs: | ||
| """Test the execution list-outputs command functions""" | ||
|
|
||
| # def get_workflow_level_outputs(config) -> dict: |
There was a problem hiding this comment.
is this a self-reminder? If so, let's be a bit explicit in the comments.
There was a problem hiding this comment.
oops, that should have been deleted
tests/unit/test_list_outputs.py
Outdated
| ], | ||
| ], | ||
| ) | ||
| def test_filer_outputs_from_workflow_metadata( |
| return filer_outputs_from_workflow_metadata(workflow_metadata) | ||
|
|
||
|
|
||
| def filer_outputs_from_workflow_metadata(workflow_metadata: dict) -> dict: |
| print(f"{i}{output_name}: {output_value}") | ||
|
|
||
|
|
||
| def is_path_or_url_like(in_string: str) -> bool: |
There was a problem hiding this comment.
I'm thinking about future-proofing us a little here: how about we classify the in_string?
I mean classify it as a local regular path, GCS path, or Azure path, or not a path at all.
There was a problem hiding this comment.
Classification would be useful in a command like fetch-outputs where we need to know the classification before downloading the output. Since we're not downloading anything in this function, we'd only be interested in knowing whether its a file.
| return requests_out.json() | ||
| else: | ||
| http_utils.check_http_request_status_code( | ||
| short_error_message="Failed to retrieve workflow outputs.", |
There was a problem hiding this comment.
It'd be better if we also include the ID of this workflow that fails.
…ain outputs else throws error. Added variable to hold workflow id in cromshellconfig.py
Add list-outputs command
Add unit and integrations tests